fix: make progress graph respect course settings #1194

RafayGhafoor · 2023-09-21T12:19:15Z

Closes #517

Description:

Currently, #517 faces an issue when the progress graph toggle is enabled/disabled but the settings are not respected, this PR requires merge of #33308 as a prerequisite so the disableProgressGraph attribute is available through course_metadata endpoint.

Testing Instructions:

Perform git pull on edx-platform repository followed by git checkout fix-progress-graph, it allows the backend to send disableProgressGraph attribute to our frontend-learning-app.
Login as edx user on studio.
Navigate to admin > waffle_utils > Waffle flag org overrides
Add and enable these flags:
Navigate to Pages and Resources Tab in frontend-course-authoring app Demo Course page and toggle Enable Progress Graph to test the settings. Make sure you save the settings to reflect the changes.

Sign out on studio and login as normal user (honor) to perform the check.
Visit demo course progress tab to see if the progress graph is respecting the configuration.

Results:

Disabled Progress Graph:

Toggle Settings:

Result:

Enabled Progress Graph:

Toggle Settings:

Result:

openedx-webhooks · 2023-09-21T12:19:20Z

Thanks for the pull request, @RafayGhafoor! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

RafayGhafoor · 2023-09-21T13:18:20Z

@Agrendalath, Please check when you have time. Thank you.

Agrendalath · 2023-09-21T14:56:10Z

@RafayGhafoor, I believe @navinkarkera is the reviewer here.

RafayGhafoor · 2023-09-21T15:52:24Z

Thank you, I will mention him here.

@navinkarkera, Please check when you time.

navinkarkera

@RafayGhafoor I have left some suggestions in edx-platform PR which will require some changes here as well.

RafayGhafoor · 2023-09-25T11:12:10Z

@navinkarkera, I have made the changes.

navinkarkera

👍 @RafayGhafoor

I tested this: (tested progress graph locally in devstack)
I read through the code
~~I checked for accessibility issues~~
~~Includes documentation~~
~~I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.~~

farhaanbukhsh

👍

✅ I tested this on my devstack and it works cleanly.
✅ I read through the code
❌ I checked for accessibility issues
❌ Includes documentation
❌ I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

itsjeyd · 2023-10-10T07:37:09Z

@RafayGhafoor Thank you for this contribution!

@Agrendalath @farhaanbukhsh Once tests are green, will we need another review from the Aurora team to be able to merge these changes, or are your approvals binding?

Agrendalath · 2023-10-10T11:52:59Z

@itsjeyd, it's a bug fix, so I suppose that we could ask the CCs of the frontend repos to merge this.

navinkarkera · 2023-10-11T04:54:59Z

@brian-smith-tcril Could you please help us triggering the tests as well as review and merge this?

codecov · 2023-10-11T14:43:31Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (168ed1e) 87.87% compared to head (c064e07) 87.91%.
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1194      +/-   ##
==========================================
+ Coverage   87.87%   87.91%   +0.04%     
==========================================
  Files         263      263              
  Lines        4485     4486       +1     
  Branches     1133     1134       +1     
==========================================
+ Hits         3941     3944       +3     
+ Misses        530      528       -2     
  Partials       14       14

Files	Coverage Δ
src/course-home/progress-tab/ProgressTab.jsx	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

brian-smith-tcril

This makes sense!

Once https://github.com/openedx/edx-platform/pull/33308/files lands, we will be getting back a boolean value for disable_progress_graph from the progress endpoint.

This MFE will grab that as a part of getProgressTabData

frontend-app-learning/src/course-home/data/api.js

Lines 223 to 224 in 5604def

    
           export async function getProgressTabData(courseId, targetUserId) { 
        
             let url = `${getConfig().LMS_BASE_URL}/api/course_home/progress/${courseId}`;

It will be camelCased and turn into disableProgressGraph

frontend-app-learning/src/course-home/data/api.js

Line 234 in 5604def

const camelCasedData = camelCaseObject(data);

And then we will be able to use it in ProgressTab

LGTM!

navinkarkera · 2023-10-11T15:10:46Z

@brian-smith-tcril I have merged openedx/edx-platform#33308, this PR can be merged now.

openedx-webhooks · 2023-10-11T15:28:15Z

@RafayGhafoor 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

(cherry picked from commit 391ea08)

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 21, 2023

RafayGhafoor marked this pull request as ready for review September 21, 2023 12:46

RafayGhafoor mentioned this pull request Sep 21, 2023

feat: Adds disable_progress_graph attribute to the returned course_me… openedx/edx-platform#33308

Merged

navinkarkera self-assigned this Sep 25, 2023

navinkarkera mentioned this pull request Sep 25, 2023

fix: await async saveSetting func to remove unwanted err msg openedx/frontend-app-authoring#610

Merged

navinkarkera suggested changes Sep 25, 2023

View reviewed changes

fix: make progress graph respect course settings

c064e07

RafayGhafoor force-pushed the progress-graph branch from b807547 to c064e07 Compare September 25, 2023 07:51

RafayGhafoor mentioned this pull request Sep 25, 2023

Can't hide progress graph from Studio openedx/frontend-app-authoring#517

Closed

RafayGhafoor requested a review from navinkarkera September 25, 2023 11:12

navinkarkera approved these changes Sep 25, 2023

View reviewed changes

RafayGhafoor changed the title ~~fix: make progress graph respect course settings~~ fix: make progress graph respect course settings. Sep 25, 2023

RafayGhafoor changed the title ~~fix: make progress graph respect course settings.~~ fix: make progress graph respect course settings Sep 26, 2023

farhaanbukhsh approved these changes Oct 5, 2023

View reviewed changes

itsjeyd added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 10, 2023

brian-smith-tcril approved these changes Oct 11, 2023

View reviewed changes

brian-smith-tcril merged commit 391ea08 into openedx:master Oct 11, 2023
5 checks passed

RafayGhafoor deleted the progress-graph branch October 11, 2023 17:48

itsjeyd removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 17, 2023

dyudyunov pushed a commit to raccoongang/frontend-app-learning that referenced this pull request Mar 4, 2024

fix: make progress graph respect course settings (openedx#1194)

89581b9

dyudyunov mentioned this pull request Mar 4, 2024

[Backport] fix: make progress graph respect course settings #1313

Merged

arbrandes pushed a commit that referenced this pull request Mar 4, 2024

fix: make progress graph respect course settings (#1194)

3b409b0

arbrandes mentioned this pull request Mar 4, 2024

completion.enable_completion_tracking waffle flag disablement doesn't remove the progress tracking UI openedx/wg-build-test-release#245

Closed

Agrendalath pushed a commit to open-craft/frontend-app-learning that referenced this pull request Oct 15, 2024

fix: make progress graph respect course settings (openedx#1194)

2765200

(cherry picked from commit 391ea08)

Agrendalath mentioned this pull request Oct 15, 2024

feat: Add support for showing and hiding parts of the progress page open-craft/frontend-app-learning#36

Open

xitij2000 added a commit to open-craft/frontend-app-learning that referenced this pull request Oct 28, 2024

fixup! fix: make progress graph respect course settings (openedx#1194)

ed5b603

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make progress graph respect course settings #1194

fix: make progress graph respect course settings #1194

RafayGhafoor commented Sep 21, 2023 •

edited

Loading

openedx-webhooks commented Sep 21, 2023 •

edited

Loading

RafayGhafoor commented Sep 21, 2023

Agrendalath commented Sep 21, 2023

RafayGhafoor commented Sep 21, 2023

navinkarkera left a comment

RafayGhafoor commented Sep 25, 2023

navinkarkera left a comment

farhaanbukhsh left a comment

itsjeyd commented Oct 10, 2023

Agrendalath commented Oct 10, 2023

navinkarkera commented Oct 11, 2023 •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

brian-smith-tcril left a comment

navinkarkera commented Oct 11, 2023

openedx-webhooks commented Oct 11, 2023

	export async function getProgressTabData(courseId, targetUserId) {
	let url = `${getConfig().LMS_BASE_URL}/api/course_home/progress/${courseId}`;

fix: make progress graph respect course settings #1194

fix: make progress graph respect course settings #1194

Conversation

RafayGhafoor commented Sep 21, 2023 • edited Loading

Description:

Testing Instructions:

Results:

Disabled Progress Graph:

Toggle Settings:

Result:

Enabled Progress Graph:

Toggle Settings:

Result:

openedx-webhooks commented Sep 21, 2023 • edited Loading

RafayGhafoor commented Sep 21, 2023

Agrendalath commented Sep 21, 2023

RafayGhafoor commented Sep 21, 2023

navinkarkera left a comment

Choose a reason for hiding this comment

RafayGhafoor commented Sep 25, 2023

navinkarkera left a comment

Choose a reason for hiding this comment

farhaanbukhsh left a comment

Choose a reason for hiding this comment

itsjeyd commented Oct 10, 2023

Agrendalath commented Oct 10, 2023

navinkarkera commented Oct 11, 2023 • edited Loading

codecov bot commented Oct 11, 2023 • edited Loading

Codecov Report

brian-smith-tcril left a comment

Choose a reason for hiding this comment

navinkarkera commented Oct 11, 2023

openedx-webhooks commented Oct 11, 2023

RafayGhafoor commented Sep 21, 2023 •

edited

Loading

openedx-webhooks commented Sep 21, 2023 •

edited

Loading

navinkarkera commented Oct 11, 2023 •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading